Skip to content

Conversation

@satheeshaGowda
Copy link
Contributor

@satheeshaGowda satheeshaGowda commented Sep 1, 2025

This change exposes an existing, persistent (in nodes.conf) unique shard Id for each shard in the cluster as part of the CLUSTER SHARDS command response.

1) 1) "slots"
   2) 1) (integer) 0
      2) (integer) 999
      3) (integer) 2001
      4) (integer) 3999
      5) (integer) 4501
      6) (integer) 5460
   3) "nodes"
   4) 1)  1) "id"
          2) "6e76043bed00e716e85035107866ea16e9a5f700"
          3) "port"
          4) (integer) 6385
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
         11) "replication-offset"
         12) (integer) 8092
         13) "health"
         14) "online"
      2)  1) "id"
          2) "b2f8c841707b2246ec2a641c37f16e88fe0bb700"
          3) "port"
          4) (integer) 6380
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
         11) "replication-offset"
         12) (integer) 8092
         13) "health"
         14) "online"
   5) "id"
   6) "3f2a7bb7bbd5fc2a331fe9bf95f5e02bcca02430"

Copy link

@nilanshu-sharma nilanshu-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@hpatro hpatro self-requested a review September 8, 2025 15:09
@zuiderkwast zuiderkwast added the major-decision-pending Major decision pending by TSC team label Sep 29, 2025
@madolson madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Oct 6, 2025
@satheeshaGowda
Copy link
Contributor Author

thanks for accepting the PR. The feedback was to change the response in to the following format instead.

1) 1) "slots"
   2) 1) (integer) 0
      2) (integer) 999
      3) (integer) 2001
      4) (integer) 3999
      5) (integer) 4501
      6) (integer) 5460
   3) "nodes"
   4) 1)  1) "id"
          2) "6e76043bed00e716e85035107866ea16e9a5f700"
          3) "port"
          4) (integer) 6385
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
         11) "replication-offset"
         12) (integer) 8092
         13) "health"
         14) "online"
      2)  1) "id"
          2) "b2f8c841707b2246ec2a641c37f16e88fe0bb700"
          3) "port"
          4) (integer) 6380
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
         11) "replication-offset"
         12) (integer) 8092
         13) "health"
         14) "online"
   5) "shard-id"
   6) "3f2a7bb7bbd5fc2a331fe9bf95f5e02bcca02430"

@hpatro
Copy link
Collaborator

hpatro commented Oct 6, 2025

I think we can just do "id".

Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
@satheeshaGowda
Copy link
Contributor Author

sample cluster shards response...

127.0.0.1:16001> cluster shards
1) 1) "slots"
   2) 1) (integer) 0
      2) (integer) 5460
   3) "nodes"
   4) 1)  1) "id"
          2) "bc22cdd1ca4e799faf01129e70cc8e617e555fba"
          3) "port"
          4) (integer) 16001
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
         11) "replication-offset"
         12) (integer) 126
         13) "health"
         14) "online"
      2)  1) "id"
          2) "abc9983ecb81d5032d321c2c681e5fb6714ad720"
          3) "port"
          4) (integer) 16004
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
         11) "replication-offset"
         12) (integer) 126
         13) "health"
         14) "online"
   5) "id"
   6) "cd7688ca959bf54a32569ccf260b26715b82a6e3"
2) 1) "slots"
   2) 1) (integer) 10923
      2) (integer) 16383
   3) "nodes"
   4) 1)  1) "id"
          2) "193bf9dee302e026232fd1e49e73db0d65344740"
          3) "port"
          4) (integer) 16003
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
         11) "replication-offset"
         12) (integer) 126
         13) "health"
         14) "online"
      2)  1) "id"
          2) "6adbdc5599872baa2498c3bf481149dec332ebed"
          3) "port"
          4) (integer) 16006
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
         11) "replication-offset"
         12) (integer) 126
         13) "health"
         14) "online"
   5) "id"
   6) "0d77234c440a6b05c2d77d1b9f37ac63585fc747"
3) 1) "slots"
   2) 1) (integer) 5461
      2) (integer) 10922
   3) "nodes"
   4) 1)  1) "id"
          2) "2a26b2d410a99207551eda195104ceed76a37e27"
          3) "port"
          4) (integer) 16002
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
         11) "replication-offset"
         12) (integer) 126
         13) "health"
         14) "online"
      2)  1) "id"
          2) "81885765e4bcc0fb0b7a198b60e6fab4dc6be991"
          3) "port"
          4) (integer) 16005
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
         11) "replication-offset"
         12) (integer) 126
         13) "health"
         14) "online"
   5) "id"
   6) "a96f023185ea6d7f5e9a0d6ada6f112741217e6a"
127.0.0.1:16001>

sample nodes.conf file that matches with shard id

 cat nodes-16001.conf
bc22cdd1ca4e799faf01129e70cc8e617e555fba 127.0.0.1:16001@26001,,tls-port=0,shard-id=cd7688ca959bf54a32569ccf260b26715b82a6e3 myself,master - 0 0 1 connected 0-5460
81885765e4bcc0fb0b7a198b60e6fab4dc6be991 127.0.0.1:16005@26005,,tls-port=0,shard-id=a96f023185ea6d7f5e9a0d6ada6f112741217e6a slave 2a26b2d410a99207551eda195104ceed76a37e27 0 1759782010000 2 connected
6adbdc5599872baa2498c3bf481149dec332ebed 127.0.0.1:16006@26006,,tls-port=0,shard-id=0d77234c440a6b05c2d77d1b9f37ac63585fc747 slave 193bf9dee302e026232fd1e49e73db0d65344740 0 1759782010000 3 connected
193bf9dee302e026232fd1e49e73db0d65344740 127.0.0.1:16003@26003,,tls-port=0,shard-id=0d77234c440a6b05c2d77d1b9f37ac63585fc747 master - 0 1759782010369 3 connected 10923-16383
2a26b2d410a99207551eda195104ceed76a37e27 127.0.0.1:16002@26002,,tls-port=0,shard-id=a96f023185ea6d7f5e9a0d6ada6f112741217e6a master - 0 1759782010000 2 connected 5461-10922
abc9983ecb81d5032d321c2c681e5fb6714ad720 127.0.0.1:16004@26004,,tls-port=0,shard-id=cd7688ca959bf54a32569ccf260b26715b82a6e3 slave bc22cdd1ca4e799faf01129e70cc8e617e555fba 0 1759782010000 1 connected
vars currentEpoch 6 lastVoteEpoch 0

Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hpatro
Copy link
Collaborator

hpatro commented Oct 6, 2025

@satheeshaGowda Could you also update cluster-shards.json file about the new field/history ?

Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
@satheeshaGowda
Copy link
Contributor Author

I think we need also update the doc site - https://github.com/valkey-io/valkey-doc/blob/main/commands/cluster-shards.md, will send that separate PR later.

@zuiderkwast zuiderkwast added needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. release-notes This issue should get a line item in the release notes labels Oct 6, 2025
Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
@hpatro hpatro changed the title Expose shard id in CLUSTER SHARDS respone Add shard id field to CLUSTER SHARDS response Oct 6, 2025
Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more thing @satheeshaGowda, forgot to mention we auto generate/update the commands.def file. Could you perform make -j and add the diff ?

Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valkey-io/core-team It's good to be merged. Do we wait for the RC3 or start cherry picking changes to 9.0 release branch from now ?

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Oct 8, 2025

Please update the top comment to match the implemenration before merging. Done. (Keeping only the description of the change that is useful as the commit message.)

@hpatro Let's just set project to 9.0, then whoever does the release will handle it.

Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
@hpatro
Copy link
Collaborator

hpatro commented Oct 8, 2025

Allowed the workflow to run the CI. It was disabled as the author is new to the project. Once that passes, I will merge it in.

@codecov
Copy link

codecov bot commented Oct 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.41%. Comparing base (0646749) to head (2017bc7).
⚠️ Report is 6 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2568      +/-   ##
============================================
+ Coverage     72.18%   72.41%   +0.22%     
============================================
  Files           128      128              
  Lines         71037    71243     +206     
============================================
+ Hits          51277    51589     +312     
+ Misses        19760    19654     -106     
Files with missing lines Coverage Δ
src/cluster_legacy.c 87.26% <100.00%> (-0.13%) ⬇️
src/commands.def 100.00% <ø> (ø)

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hpatro hpatro merged commit 5bbbc6b into valkey-io:unstable Oct 8, 2025
53 checks passed
@github-project-automation github-project-automation bot moved this to To be backported in Valkey 9.0 Oct 8, 2025
@madolson
Copy link
Member

madolson commented Oct 8, 2025

We released 9.0 RC3 without this (I wasn't aware that this was pending). I think we should punt this back to 9.1.

@hpatro
Copy link
Collaborator

hpatro commented Oct 8, 2025

We released 9.0 RC3 without this (I wasn't aware that this was pending). I think we should punt this back to 9.1.

I wasn't sure as well. @zuiderkwast suggested to target it to 9.0. Do we still want to do it with GA or just punt ?

@zuiderkwast
Copy link
Contributor

Idk. I thought we talked about it in the last meeting to be able to squeeze it in for 9.0, but I may have misunderstood. I suppose we shouldn't add new features after rc1 (or after rc2 for small ones maybe).

@madolson
Copy link
Member

madolson commented Oct 9, 2025

I suppose we shouldn't add new features after rc1 (or after rc2 for small ones maybe).

No new features after rc1, and no incremental features after rc2 is sort of my belief.

@hpatro hpatro removed this from Valkey 9.0 Oct 13, 2025
@hpatro hpatro moved this to Done in Valkey 9.1 Oct 13, 2025
roshkhatri pushed a commit to roshkhatri/valkey that referenced this pull request Oct 14, 2025
This change exposes an existing, persistent (in nodes.conf) unique shard
identifier for each shard in the cluster as part of the `CLUSTER SHARDS` command
response.

```
1) 1) "slots"
   2) 1) (integer) 0
      2) (integer) 999
      3) (integer) 2001
      4) (integer) 3999
      5) (integer) 4501
      6) (integer) 5460
   3) "nodes"
   4) 1)  1) "id"
          2) "6e76043bed00e716e85035107866ea16e9a5f700"
          3) "port"
          4) (integer) 6385
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
         11) "replication-offset"
         12) (integer) 8092
         13) "health"
         14) "online"
      2)  1) "id"
          2) "b2f8c841707b2246ec2a641c37f16e88fe0bb700"
          3) "port"
          4) (integer) 6380
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
         11) "replication-offset"
         12) (integer) 8092
         13) "health"
         14) "online"
   5) "id"
   6) "3f2a7bb7bbd5fc2a331fe9bf95f5e02bcca02430"
```

---------

Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
Co-authored-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
hpatro added a commit to valkey-io/valkey-doc that referenced this pull request Oct 15, 2025
Related to the  PR valkey-io/valkey#2568

---------

Signed-off-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Satheesha Chattenahalli Hanume Gowda <[email protected]>
Co-authored-by: Harkrishn Patro <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-decision-approved Major decision approved by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. release-notes This issue should get a line item in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants